Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow CW->ERC pointers to be called through wasmd precompile #1785

Merged
merged 1 commit into from
Aug 13, 2024

Conversation

codchen
Copy link
Collaborator

@codchen codchen commented Jul 26, 2024

Describe your changes and provide context

For EVM transactions that call the wasmd precompile (0x1002) directly, we want to allow the destination CW contract to be call back to some EVM contracts (e.g. ERC20) at most once (right now none is allowed). This PR leverages a new flag set on sdk.Context to track whether a CW->EVM call is eligible for this special case. Note that this only applies to the top level - something like precompile->CW->precompile->CW is still not allowed.

Testing performed to validate your change

hardhat test

@codchen codchen force-pushed the tony-chen-handle-wasmd-call branch 3 times, most recently from 181e4fb to ab68883 Compare July 29, 2024 12:14
Copy link

codecov bot commented Jul 29, 2024

Codecov Report

Attention: Patch coverage is 51.61290% with 30 lines in your changes missing coverage. Please review.

Project coverage is 60.81%. Comparing base (e8e4b3b) to head (4ee4fc2).
Report is 136 commits behind head on main.

Files Patch % Lines
x/evm/keeper/msg_server.go 16.00% 19 Missing and 2 partials ⚠️
x/evm/keeper/evm.go 83.33% 2 Missing and 1 partial ⚠️
x/evm/keeper/receipt.go 0.00% 3 Missing ⚠️
precompiles/wasmd/wasmd.go 71.42% 2 Missing ⚠️
evmrpc/simulate.go 88.88% 1 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1785      +/-   ##
==========================================
- Coverage   61.64%   60.81%   -0.83%     
==========================================
  Files         365      380      +15     
  Lines       26178    28021    +1843     
==========================================
+ Hits        16138    17042     +904     
- Misses       8967     9825     +858     
- Partials     1073     1154      +81     
Files Coverage Δ
evmrpc/simulate.go 64.82% <88.88%> (-3.09%) ⬇️
precompiles/wasmd/wasmd.go 60.62% <71.42%> (-0.60%) ⬇️
x/evm/keeper/evm.go 68.18% <83.33%> (+15.69%) ⬆️
x/evm/keeper/receipt.go 67.59% <0.00%> (-0.83%) ⬇️
x/evm/keeper/msg_server.go 70.22% <16.00%> (-8.60%) ⬇️

... and 75 files with indirect coverage changes

@codchen codchen force-pushed the tony-chen-handle-wasmd-call branch 2 times, most recently from de1c3bf to 02da59a Compare July 30, 2024 04:02
@@ -118,6 +118,23 @@ func (k *Keeper) CallEVM(ctx sdk.Context, from common.Address, to *common.Addres
if res.Err != nil {
vmErr = res.Err.Error()
}
existingReceipt, err := k.GetTransientReceipt(ctx, ctx.TxSum())
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this logic related to the wasmd precompile stuff?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah it's to merge with the temp receipts created in the CW->EVM hop

Copy link
Contributor

@jewei1997 jewei1997 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

overall LGTM

@codchen codchen force-pushed the tony-chen-handle-wasmd-call branch from 02da59a to 824831c Compare August 13, 2024 03:22
@codchen codchen force-pushed the tony-chen-handle-wasmd-call branch from 824831c to 4ee4fc2 Compare August 13, 2024 05:08
@codchen codchen merged commit fc36b39 into main Aug 13, 2024
49 of 50 checks passed
@codchen codchen deleted the tony-chen-handle-wasmd-call branch August 13, 2024 15:05
yzang2019 added a commit that referenced this pull request Aug 15, 2024
* main:
  Add more DEX dapp tests (#1809)
  Add basic LST integration tests  (#1814)
  Allow CW->ERC pointers to be called through wasmd precompile (#1785)
  Bump nonce even if tx fails (#1778)
  Fix docker setup for local cluster (#1806)
  Tune Configs (#1813)
  V5.7.5 release (#1805)
  Evidence Max Bytes Update (#1812)
  Add dApp Tests (#1802)
  [EVM] Tune configs (#1800)
  Revert dex removal (#1801)
  Do not charge gas for feecollector address query (#1795)
@ARitz-Cracker
Copy link

Apologies for being late to this. I received no communication that this was the implemented solution until now.

What exactly does this protect against? I can deploy an "ERC-20 token" which can still embed a nasty payload if the "transfer" function was called with specific sender and use the address and amount params as commands for the malicious code.

This solution needlessly impedes the ability for CW contracts to call the ERC-20 tokens directly without the use of a pointer with no security benefit.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants